[BOLT][AArch64] Treat br x30 as a return
#159458
Conversation
On AArch64, `br x30` instruction is equivalent to `ret`. Treat it as such while analysing the control flow. Without the special case, `br x30` is considered an indirect branch with an unknown control flow making a control flow analysis more complicated than necessary. We can also replace `br x30` with `ret`. I'd rather do it in a separate pass to allow tools built on top of BOLT to have access to the original assembly.
maksfb
requested review from
aaupov,
rafaelauler,
ayermolo,
yota9,
paschalis-mpeis and
yozhu
as code owners
|
@llvm/pr-subscribers-bolt Author: Maksim Panchenko (maksfb) ChangesOn AArch64, Without the special case, We can also replace Full diff: https://github.com/llvm/llvm-project/pull/159458.diff 2 Files Affected:
diff --git a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
index f972646aa12ea..b8da5d854b836 100644
--- a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
+++ b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp
@@ -152,6 +152,27 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
return isLoadFromStack(Inst);
};
+ bool isReturn(const MCInst &Inst) const override {
+ // BR X30 is equivalent to RET
+ if (Inst.getOpcode() == AArch64::BR &&
+ Inst.getOperand(0).getReg() == AArch64::LR)
+ return true;
+
+ return Analysis->isReturn(Inst);
+ }
+
+ bool isBranch(const MCInst &Inst) const override {
+ if (isReturn(Inst))
+ return false;
+ return Analysis->isBranch(Inst);
+ }
+
+ bool isIndirectBranch(const MCInst &Inst) const override {
+ if (isReturn(Inst))
+ return false;
+ return Analysis->isIndirectBranch(Inst);
+ }
+
void createCall(MCInst &Inst, const MCSymbol *Target,
MCContext *Ctx) override {
createDirectCall(Inst, Target, Ctx, false);
diff --git a/bolt/test/AArch64/br-x30.s b/bolt/test/AArch64/br-x30.s
new file mode 100644
index 0000000000000..711e8a8faef84
--- /dev/null
+++ b/bolt/test/AArch64/br-x30.s
@@ -0,0 +1,16 @@
+## Test that "br x30" is treated as a return instruction and not as an indirect
+## branch.
+
+# RUN: %clang %cflags %s -o %t.exe -Wl,-q -Wl,--entry=foo
+# RUN: llvm-bolt %t.exe -o %t.bolt --print-cfg 2>&1 | FileCheck %s
+
+# CHECK: BB Count : 2
+# CHECK-NOT: UNKNOWN CONTROL FLOW
+
+ .text
+ .global foo
+ .type foo, %function
+foo:
+ br x30
+ nop
+ .size foo, .-foo
|
|
Hmmm.... There are differences between the semantics of Also for BTI (which is already widely deployed), there is a difference. whereas the As GCS (guarded control stack) gets deployed more widely, I would assume that most code that uses |
There was a problem hiding this comment.
RET does BR x30 by default, but a BR x30 is not necessarily a return.
In the PCS, x30 is the LR, but it can still be used as a GP in custom assembly.
Do you see
BR x30frequently enough in code in the wild that it makes a meaningful difference to recognize it as a return in BOLT?
It'll be interesting to see where such cases comes from, and as Kristof says, how often they occur.
At minimum, we could emit some warnings to increase visibility and help improve the input sources (where is needed), or guard this simplification behind an experimental flag?
|
Thanks for the details on the differences between the two instructions. For the part where we decide how to treat For instruction conversion, which I don't yet plan to implement, it makes sense to perform such only under an option and issue a warning. As to how frequent |
|
|
On AArch64,
br x30instruction is equivalent toret. Treat it as such while analysing the control flow.Without the special case,
br x30is considered an indirect branch with an unknown control flow making a control flow analysis more complicated than necessary.We can also replace
br x30withret. I'd rather do it in a separate pass to allow tools built on top of BOLT to have access to the original assembly.